xDS unified mux: subscription states by dmitri-d · Pull Request #16486 · envoyproxy/envoy

dmitri-d · 2021-05-13T16:55:27Z

Split out subscription state out of xDS mux unification PR (#15473). Made base subscription state class a template. Updated existing delta state tests to work with both legacy and new implementations.

Commit Message: xDS unified mux: subscription states
Additional Description:
Risk Level: low, the code is not being used atm
Testing: updated existing tests, added new ones
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2021-05-13T16:56:58Z

Ping @adisuissa. I followed your suggestion re: how to split the original PR (#15473).

adisuissa · 2021-05-13T17:05:01Z

/assign @adisuissa

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

…t.cc Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

adisuissa

Thanks for improving this code.
I've left a few comments, mostly about possible refactoring that I think can be done.

source/common/config/unified_mux/delta_subscription_state.cc

adisuissa · 2021-05-17T02:23:24Z

source/common/config/unified_mux/delta_subscription_state.cc

+envoy::service::discovery::v3::DeltaDiscoveryRequest*
+DeltaSubscriptionState::getNextRequestInternal() {
+  auto* request = new envoy::service::discovery::v3::DeltaDiscoveryRequest;
+  request->set_type_url(type_url());


previously, in addition to type_url, the request's node field was also updated. Why was this removed here?

Also it seems that this is something that should be generic for both Delta/SotW variants, and should be set in the parent class.

This bit is handled by mux (here: https://github.com/envoyproxy/envoy/pull/15473/files#diff-fe56236f0649ba734162e4fa294465a6004c393242c0af89421cbd5fe279fe58R341 and here: https://github.com/envoyproxy/envoy/pull/15473/files#diff-fe56236f0649ba734162e4fa294465a6004c393242c0af89421cbd5fe279fe58R398) in the new implementation, although I think you are right, conceptually it fits better with the state. I'd like to postpone this change until the PR that introduces new mux implementation, however: it's a bit tricky to keep track of changes that affect multiple classes and more importantly the logic inside. I can add a comment to that effect?

In current implementation sotw and delta muxes handle node differently (delta always sends node), the new implementation uses the approach currently used by sotw mux in both cases.

source/common/config/unified_mux/subscription_state.h

adisuissa · 2021-05-17T02:43:11Z

source/common/config/unified_mux/subscription_state.h

+  // pointer actually is.
+  virtual RQ* getNextRequestWithAck(const UpdateAck& ack) PURE;
+
+  void disableInitFetchTimeoutTimer() {


It seems that enabling the timer only happens during the object initialization, and then disable can be called, and there's no way to re-enable the timer. Is this intended behavior? Can you add a comment why it behaves so?

This only handles initial fetch timeout. Currently this is handled in the subscription (here: https://github.com/envoyproxy/envoy/blob/main/source/common/config/grpc_subscription_impl.cc#L31), but has been moved to subscription state in the new implementation. Added a comment, please let me know if it's descriptive enough.

What's the rationale for the move?

It was moved in the original version of this work done by Fred, and I thought it was a good call. I just noticed though that this introduced a problem under Ads: should any subscription sharing mux with other subscriptions call GrpcMux::disableInitFetchTimeoutTimer() (here: 109ac32#diff-fe56236f0649ba734162e4fa294465a6004c393242c0af89421cbd5fe279fe58R183), all subscriptions will get their init fetch timeout timers disabled. I don't think there's a way around this, I'll move the timer and corresponding calls back to subscription.

adisuissa · 2021-05-17T02:47:16Z

source/common/config/unified_mux/subscription_state.h

+  void setDynamicContextChanged() { dynamic_context_changed_ = true; }
+  void clearDynamicContextChanged() { dynamic_context_changed_ = false; }
+  bool dynamicContextChanged() const { return dynamic_context_changed_; }


Should these be protected instead of public?

Probably not atm: these are called from mux (here: https://github.com/envoyproxy/envoy/pull/15473/files#diff-fe56236f0649ba734162e4fa294465a6004c393242c0af89421cbd5fe279fe58R41 and https://github.com/envoyproxy/envoy/pull/15473/files#diff-fe56236f0649ba734162e4fa294465a6004c393242c0af89421cbd5fe279fe58R351)

source/common/config/unified_mux/delta_subscription_state.cc

source/common/config/unified_mux/sotw_subscription_state.cc

adisuissa · 2021-05-17T03:21:39Z

source/common/config/unified_mux/delta_subscription_state.cc

+  Protobuf::RepeatedPtrField<std::string> removed_resources;
+  for (const auto& resource : expired) {
+    resource_state_[resource] = ResourceState::waitingForServer();
+    removed_resources.Add(std::string(resource));


Why is the call to the string c'tor required here?

This is a port from here: https://github.com/envoyproxy/envoy/blob/main/source/common/config/delta_subscription_state.cc#L22. My understaning is that Protobuf::RepeatedPtrField takes ownership of strings/massages added to it, hence the signature (RepeatedPtrField<T>::Add(T&& value)) and the need for a new instance of the string.

source/common/config/unified_mux/subscription_state.h

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

adisuissa

Looks good, thanks!
One more comment about the tests.

adisuissa · 2021-05-21T15:46:42Z

test/common/config/delta_subscription_state_test.cc

+  std::unique_ptr<Envoy::Config::DeltaSubscriptionState> legacy_state_;
+  std::unique_ptr<Envoy::Config::UnifiedMux::DeltaSubscriptionState> unified_state_;


Should these be a variant?
If there's no use of both at the same time, I think it makes sense.

adisuissa · 2021-05-24T10:51:34Z

/cc @htuch
I think the PR looks good.

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2021-05-25T23:54:22Z

responded to feedback

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2021-05-26T21:40:08Z

increased test coverage

dmitri-d · 2021-05-26T22:58:24Z

Not sure why the coverage for source/common/config/unified_mux is reported so low; I have it at 96.5% locally.

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

alyssawilk · 2021-06-02T17:21:45Z

If you check https://github.com/envoyproxy/envoy/blob/main/source/docs/coverage.md it should show you what's missing
there are some quirks to coverage you may be hitting?

alyssawilk · 2021-06-02T17:22:50Z

Yan: can you cover code reviews until @htuch gets back?

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2021-06-02T23:41:12Z

@adisuissa: discovered an issue with how I was handling ttl resources in SotW; fixed now. Also expanded test coverage.

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

source/common/config/unified_mux/delta_subscription_state.cc

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2021-06-10T22:19:01Z

ping @yanavlasov

dmitri-d · 2021-06-11T20:25:24Z

@htuch: any chance you could take a look?

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch

Thanks @dmitri-d, this looks great! Thanks for breaking out the mega PR. This mostly looks good to me, but a few comments here and there to get started.
/wait

source/common/config/unified_mux/subscription_state.h

htuch · 2021-06-18T19:09:25Z

source/common/config/unified_mux/subscription_state.h

+class SubscriptionState : public Logger::Loggable<Logger::Id::config> {
+public:
+  // Note that, outside of tests, we expect callbacks to always be a WatchMap.
+  SubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& callbacks,


Nit: const std::string& or absl::string_view

please see the comment below.

source/common/config/unified_mux/subscription_state.h

htuch · 2021-06-18T19:14:33Z

source/common/config/unified_mux/subscription_state.h

+  // pointer actually is.
+  virtual RQ* getNextRequestWithAck(const UpdateAck& ack) PURE;
+
+  void disableInitFetchTimeoutTimer() {


What's the rationale for the move?

htuch · 2021-06-18T19:15:55Z

source/common/config/unified_mux/delta_subscription_state.h

+    : public SubscriptionState<envoy::service::discovery::v3::DeltaDiscoveryResponse,
+                               envoy::service::discovery::v3::DeltaDiscoveryRequest> {
+public:
+  DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map,


Nit: can you check all the places you are passing strings as args and move them to refs/string_views?

I checked string parameters in ctors. In ctor implementations, the initial copy is being moved all the way to the base class member initialization (here: https://github.com/envoyproxy/envoy/pull/16486/files#diff-807032428ceb00bb04de1be63707b2ae3dccaa00912341208fd4b8da55993b17R38).

I guess this is OK, but it's a bit counter-intuitive to someone reading the interface. Usually prefer to pass by reference and then leave the copy internally.

source/common/config/unified_mux/delta_subscription_state.cc

source/common/config/unified_mux/sotw_subscription_state.h

source/common/config/unified_mux/sotw_subscription_state.cc

test/common/config/delta_subscription_state_test.cc

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2021-07-06T23:14:44Z

merged changes from main
responded to feedback

htuch

LGTM, thanks!

Split out subscription state out of xDS mux unification PR (envoyproxy#15473). Made base subscription state class a template. Updated existing delta state tests to work with both legacy and new implementations. Risk Level: low, the code is not being used atm Testing: updated existing tests, added new ones Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

xDS unified mux: subscription states

4f4da67

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d changed the title ~~Part 1 of unified mux: subscription states~~ xDS unified mux: subscription states May 13, 2021

repokitteh-read-only bot assigned adisuissa May 13, 2021

Dmitri Dolguikh added 2 commits May 14, 2021 10:20

Fixed formatting issues

a354a86

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Reverted an inadvertent change in test/server/admin/stats_handler_tes…

960f179

…t.cc Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

adisuissa reviewed May 17, 2021

View reviewed changes

Dmitri Dolguikh added 2 commits May 17, 2021 19:07

Responded to feedback

cd5cad2

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

making clang_tidy happy

0cc40ee

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

adisuissa reviewed May 24, 2021

View reviewed changes

Switched to use absl::variant

815925e

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Increased test coverage

7d5b5e7

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Merge branch 'main' into xds-unification-part-1

2233191

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

alyssawilk assigned yanavlasov Jun 2, 2021

Fixed handling of resources with ttl in sotw; increased test coverage

f821dba

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Dmitri Dolguikh added 7 commits June 3, 2021 11:02

Expanded test coverage

81457b4

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Merge remote-tracking branch 'upstream/main' into xds-unification-part-1

1058555

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixed a bug in SotW subscription test

146e2ac

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Merge remote-tracking branch 'upstream/main' into xds-unification-part-1

ec6a57a

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Updated include paths

c182b6f

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixed formatting

ad6d14b

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Merge remote-tracking branch 'upstream/main' into xds-unification-part-1

f5cbfa8

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Fixed build failure

e878e9a

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

yanavlasov reviewed Jun 8, 2021

View reviewed changes

source/common/config/unified_mux/delta_subscription_state.cc Show resolved Hide resolved

dmitri-d mentioned this pull request Jun 9, 2021

WIP: xDS mux unification #15473

Closed

yanavlasov previously approved these changes Jun 9, 2021

View reviewed changes

Added todos re: copying of resources

76b631e

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d dismissed yanavlasov’s stale review via 76b631e June 9, 2021 18:37

Merge remote-tracking branch 'upstream/main' into xds-unification-part-1

0661ed4

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Merge remote-tracking branch 'upstream/main' into xds-unification-part-1

6328d9d

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch suggested changes Jun 18, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 18, 2021

dmitri-d mentioned this pull request Jun 18, 2021

WIP: xds mux unification part 2 #17049

Closed

Dmitri Dolguikh added 2 commits July 5, 2021 15:03

Merge branch 'main' into xds-unification-part-1

f0fd66b

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Responded to feedback

db66b92

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

repokitteh-read-only bot removed the waiting label Jul 6, 2021

Fixed formatting

da6d49f

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch approved these changes Jul 9, 2021

View reviewed changes

htuch merged commit eb3658a into envoyproxy:main Jul 9, 2021

		std::unique_ptr<Envoy::Config::DeltaSubscriptionState> legacy_state_;
		std::unique_ptr<Envoy::Config::UnifiedMux::DeltaSubscriptionState> unified_state_;

Conversation

dmitri-d commented May 13, 2021

Uh oh!

dmitri-d commented May 13, 2021

Uh oh!

adisuissa commented May 13, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitri-d Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa commented May 24, 2021

Uh oh!

dmitri-d commented May 25, 2021

Uh oh!

dmitri-d commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitri-d commented May 26, 2021

Uh oh!

alyssawilk commented Jun 2, 2021

Uh oh!

alyssawilk commented Jun 2, 2021

Uh oh!

dmitri-d commented Jun 2, 2021

Uh oh!

Uh oh!

dmitri-d commented Jun 10, 2021

Uh oh!

dmitri-d commented Jun 11, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitri-d Jun 22, 2021 •

edited

Loading

dmitri-d commented May 26, 2021 •

edited

Loading